428 dynamic bounds margin #467
Conversation
There was a problem hiding this comment.
Hey I went through the code the initial work looks good, however I've found some points I would like to see added / changed:
For the Stage.ts:
- Set the default boundsMargin of the stage when creating the rootNode.
- Add getter/setter for boundsMargin on Stage and have it also adjust the boundsMargin of the rootNode.
For the CoreNode:
- for
get boundsMarginI don't think we need to check if the boundsMargin in the props is an Array or not, just check if this.props.boundsMargin is not null and return that value. If it is null let it check the parent node(s) with the changes to Stage we won't need the stage.boundsMargin - In case the boundsMargin for a CoreNode is set upon creating the node we should call the boundsMargin setter like we are doing with the shaders/texture (line: 761)
|
Added dynamic boundsmargin option to stage, also boundsmargin can be set to null, so it can take over parents boundsmargin or stages boundsmargin. Went throught the changes requested by @jfboeve |
looks good, one Q though: should we really introduce a new updateType? The RenderBound could also update its children, which might be a better pattern overall. Currently the same behaviour exists on RenderBound whenever clipping is enabled or at the global position recalculation when clipping is enabled. Can be replaced by using to behave the same as your suggested |
|
I agree with you @wouterlucas since the renderBounds are calculated based on boundsMargin it's better to add it to the RenderBound |
…nly the first) And use renderBounds updateType
|
I had it seperate because the renderBounds did only propagate 1 child up. |
| @@ -193,7 +193,7 @@ export enum UpdateType { | |||
| /** | |||
| * All | |||
| */ | |||
| All = 14335, | |||
| All = 16383, | |||
There was a problem hiding this comment.
left over from the new type?
Added option to set boundsmargin on node level.
Also able to change it after creation.